fix(system): correct service, update repair, and legacy imports - #4966
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes correct delayed service startup configuration, replace WSUS registry deletion processes with PowerShell commands, and improve legacy import handling with atomic selection replacement and regression coverage. ChangesWindows utility maintenance
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR updates service repair, Windows Update repair, and legacy import behavior with focused and full test validation reported as passing; no actionable merge-blocking risk remains. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@functions/private/Set-WinUtilService.ps1`:
- Line 36: Update the service configuration flow around sc.exe config in
Set-WinUtilService so it checks $LASTEXITCODE immediately after the command and
throws when the exit code is non-zero. Keep the success log only on the
successful path, allowing the existing catch handling to report configuration
failures.
In `@functions/public/Invoke-WPFImpex.ps1`:
- Around line 76-86: The legacy-import flattening logic around $flattenedJson
must exclude the metadata object represented by the Install property before
Update-WinUtilSelections is called, retaining only selection identifiers and
preserving flat-import behavior. Add a regression fixture that verifies legacy
and flat imports produce equivalent selections.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b64fa5d-4c41-4263-b7f5-9ee0229a183c
📒 Files selected for processing (4)
functions/private/Set-WinUtilDNS.ps1functions/private/Set-WinUtilService.ps1functions/public/Invoke-WPFFixesUpdate.ps1functions/public/Invoke-WPFImpex.ps1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60a541d1cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
Thanks for working on this. I raised the same concern on #4911, but a decision has not been made in #4877 about what backward compatibility should mean here. The backup format changed on February 17, almost six months ago. It is up to Chris to decide whether full compatibility is necessary or partial import is acceptable. This skips the old I also cannot verify this was tested. There is no legacy JSON fixture, import-path test, or example of the old input and expected selections. Finally, every provider in |
I'd say keep the legacy import implementation for now and lets wait for Chris' input. The IPv6 guard can be removed. Good work on the bugfixes 😄 |
|
sounds good, removed the ipv6 guard commit as suggested! leaving the rest for chris to review 👍 |
ChrisTitusTech
left a comment
There was a problem hiding this comment.
Reviewed the final branch after syncing with main. Local compile and all 562 Pester tests pass, all review threads are resolved, and both Codex and CodeRabbit review loops have no remaining actionable findings.
Summary
sc.exewith the required spacing and report non-zero exit codesREG DELETEpathsInstallpackage metadata and retaining strict validation for selection keysmainbranch and add regression coverage for the historical import shapeValidation
.\Compile.ps1Manual validation
The destructive service and Windows Update repair operations were not run against the host. Their behavior is covered by source review and focused helper tests.